Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add documentation for SetQueryNotification #524

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

chris-rossi
Copy link
Contributor

Add Godoc documentation for the SetQueryNotification feature.

@codecov
Copy link

codecov bot commented Sep 24, 2019

Codecov Report

Merging #524 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
+ Coverage   68.84%   68.86%   +0.01%     
==========================================
  Files          22       22              
  Lines        5056     5058       +2     
==========================================
+ Hits         3481     3483       +2     
  Misses       1369     1369              
  Partials      206      206
Impacted Files Coverage Δ
mssql.go 82.72% <ø> (ø) ⬆️
conn_str.go 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 208c0a4...8638508. Read the comment docs.

@@ -0,0 +1,61 @@
package main
Copy link
Contributor

@yukiwongky yukiwongky Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only documentation (md files) should be in the doc folder. If this example test works as a standalone test, please put it in the root folder.

Since this example is targeting the SetQueryNotification, you should rename is to something like setquerynotification_example_text.go.

If you look at other *_examples_test.go files (e.g., datetimeoffset_example_test.go), you will see that instead of identifying is as package main, they use package mssql_test. That way if this file is in the root folder along with the src files, you will not get package clash (having 2 packages in the same directory). When godoc gets this example, they'll replace package mssql_test with package main so it looks like a proper test application.

Suggested change
package main
package mssql_test

It's also a good idea to add a .md file in the doc folder to show the users now this query notification feature works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the function has "Example" prefixing it's name it won't be run as a test. I have moved it from the doc folder though.

"log"
"time"

mssql "github.com/denisenkom/go-mssqldb"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to explicitly set the alias. It is mssql by default.

Suggested change
mssql "github.com/denisenkom/go-mssqldb"
"github.com/denisenkom/go-mssqldb"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept it since it seems to be like this in other examples.

server = flag.String("server", "", "the database server")
user = flag.String("user", "", "the database user")
database = flag.String("database", "", "the database name")
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will get redefinition errors here if this file is in package mssql_test, since these variables are already defined in newconnector_example_test.go. You can just remove this whole block of variables.

conn, _ := cn.(*mssql.Conn)

// Supported SELECT statements: https://docs.microsoft.com/en-us/previous-versions/sql/sql-server-2008-r2/ms181122(v=sql.105)
stmt, err := conn.Prepare("SELECT [myColumn] FROM [mySchema].[myTable];")
Copy link
Contributor

@yukiwongky yukiwongky Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any special setup needed for the Schema and Table for query notification to work? Perhaps add comments letting user know what setup is required?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no special setup for the schema or table but query notifications must be setup on SQL Server for this example to work.

if err != nil {
log.Fatal("Query failed:", err.Error())
} else {
rows.Close()
Copy link
Contributor

@yukiwongky yukiwongky Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does rows contain in this case? Perhaps add a comment letting user know what is the expected output?

} else {
rows.Close()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline.

@kardianos
Copy link
Collaborator

@chris-rossi and @yukiwongky Thank you for this documentation.

Upon examining this feature and how it is used, I would strongly like to alter the current implementation unless I'm totally missing something.

Recent database/sql package has a Conn.Raw function, which is our key to functionality like this. https://pkg.go.dev/database/sql?tab=doc#Conn.Raw

I would like to come up with an alternate API for using query notifications that do not involve bypassing the connection pool. First, is it required to set it on a Stmt? In SQL Server, a query batch is first class, not a Stmt. Second, I would like an API like:

package mssql

type QueryNotificationOptions {
    Message string
    Options string // map[string]string?
    Timeout time.Duration
}

// SetQueryNotification may be used on a *sql.Conn prior to a query to receive notifications
// on that query until the given timeout.
//
//    conn, err := db.Conn(ctx)
//    err = mssql.SetQueryNotifications(conn, opts)
//    rows, err := conn.QueryContext(ctx, `select * from notifications;`)
func SetQueryNotification(conn *sql.Conn, opts QueryNotificationOptions) error {
    return conn.Raw(func(ci interface{}) error {
        msc, ok := ci.(*mssql.Conn)
        if !ok {
            return fmt.Errorf("expected connection to be of type *mssql.Conn, got %T", ci)
        }
        msc.SetQueryNotification(opts)
    })
}

@chris-rossi
Copy link
Contributor Author

@kardianos I don't see any reason why this wouldn't work. I think initially they designed it to be set in Stmt because it would be need to changed on a query to query basis but as long as they change the "NotifyId" (Message) for each query they want a subscription for. We would need to something that would clear the QueryNotificationOptions for the cases where they don't want a query notification subscription.

@kardianos
Copy link
Collaborator

@chris-rossi Then maybe the signature would be:

type Notification struct {...}
func (n *Notification) QueryContext(ctx context.Context, query string, ...interface{}) (*sql.Rows, error)
func (n *Notification) Cancel(ctx context.Context) error
func (n *Notification) ID() string
func SetQueryNotification(conn *sql.Conn, opts QueryNotificationOptions) (*Notification, error)

?

@chris-rossi
Copy link
Contributor Author

I presume ID() will return the NotifyID. This looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants